fix: fix possible panic when 'SetNode' is called #1685

eggiter · 2021-08-19T12:35:14Z

This commit fixes possible panic when SetNode is called, which may happen when gpus of one machine are lost during the restarting of nvidia-device-plugin;
Detailed reproduce procedures are provided in UT TestNodeInfo_SetNode;
This commit is a supplement to MR Fix the scheduler panic whenever the GPU is lost on node #306 and issue Scheduler panic happens when the GPU is lost on node #294.

eggiter · 2021-08-19T12:36:15Z

/assign @william-wang @k82cn @hzxuzhonghu

Thor-wl · 2021-08-23T11:41:51Z

pkg/scheduler/api/resource_info.go

@@ -187,10 +187,14 @@ func (r *Resource) Add(rr *Resource) *Resource {
 	return r
 }

-//Sub subtracts two Resource objects.
+// Sub subtracts two Resource objects with assertion.


Although this modification is clear, I think the original function is also acceptable.

Thor-wl · 2021-08-23T11:42:06Z

/lgtm

eggiter · 2021-08-27T09:00:05Z

@william-wang @k82cn @hzxuzhonghu
PTAL!

k82cn · 2021-08-27T23:34:32Z

/hold

This commit fixes possible panic when SetNode is called, which may happen when gpus of one machine are lost during the restarting of nvidia-device-plugin;

Is there any panic stack info for this issue?

hzxuzhonghu · 2021-08-28T02:42:17Z

pkg/scheduler/api/node_info.go

+	}
+
+	// Dry run, make sure all fields other than `State` are in the original state.
+	copy := ni.Clone()


To make make sure all fields are left untouched and in the original state, as i commented.

Can you point where do we mutate this node?

The original codes are all changing fields of nodeInfo.

Here, the clone i used, is merely a dry-run to make sure if all these changes makes this node not ready then we shall not to actually do this operation and just set state of this node to notready.

eggiter · 2021-08-30T03:45:57Z

@k82cn

Stack traces are listing as follows;
Detailed re-produce procedures can be found in UT TestNodeInfo_SetNode;

panic: resource is not sufficient to do operation: <cpu 5000.00, memory 5000000000.00> sub <cpu 6000.00, memory 6000000000.00> [recovered]
	panic: resource is not sufficient to do operation: <cpu 5000.00, memory 5000000000.00> sub <cpu 6000.00, memory 6000000000.00>

goroutine 28 [running]:
testing.tRunner.func1.1(0x278c940, 0xc00007f290)
	/usr/local/go/src/testing/testing.go:1072 +0x552
testing.tRunner.func1(0xc000182600)
	/usr/local/go/src/testing/testing.go:1075 +0x328
panic(0x278c940, 0xc00007f290)
	/usr/local/go/src/runtime/panic.go:975 +0x4c6
volcano.sh/volcano/pkg/scheduler/util/assert.Assert(0x29e9a00, 0xc00044c200, 0x77)
	/workspace/go/src/volcano.sh/volcano/pkg/scheduler/util/assert/assert.go:33 +0x205
volcano.sh/volcano/pkg/scheduler/util/assert.Assertf(0xc0004d6600, 0x29e9a8c, 0x39, 0xc0004e7898, 0x2, 0x2)
	/workspace/go/src/volcano.sh/volcano/pkg/scheduler/util/assert/assert.go:43 +0xa5
volcano.sh/volcano/pkg/scheduler/api.(*Resource).Sub(0xc0004d67a0, 0xc0004d6620, 0x0)
	/workspace/go/src/volcano.sh/volcano/pkg/scheduler/api/resource_info.go:192 +0x15e
volcano.sh/volcano/pkg/scheduler/api.(*NodeInfo).SetNode(0xc000318e70, 0xc0004ea300)
	/workspace/go/src/volcano.sh/volcano/pkg/scheduler/api/node_info.go:329 +0x5a5
volcano.sh/volcano/pkg/scheduler/api.TestNodeInfo_SetNode(0xc000182600)
	/workspace/go/src/volcano.sh/volcano/pkg/scheduler/api/node_info_test.go:222 +0xda5
testing.tRunner(0xc000182600, 0x2a50500)
	/usr/local/go/src/testing/testing.go:1123 +0x1a3
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1168 +0x648

hzxuzhonghu · 2021-08-31T07:33:07Z

@eggiter we need to fix func (r *Resource) Sub(rr *Resource) *Resource {

hzxuzhonghu · 2021-08-31T07:33:49Z

Amazing there is panic in the functiuon, should remove all the stuff

k82cn · 2021-08-31T07:40:24Z

Detailed re-produce procedures can be found in UT TestNodeInfo_SetNode;

AFAIK, we'll ignore not ready Nodes; is there any e2e case to trigger the panic?

eggiter · 2021-08-31T08:03:00Z

Detailed re-produce procedures can be found in UT TestNodeInfo_SetNode;

AFAIK, we'll ignore not ready Nodes; is there any e2e case to trigger the panic?

Yes, nodes which are not ready are already ignored.
But this situation here is:
- the node is Ready but the allocatable gpu becomes '0'.
- furthermore, this event triggers NodeUpdate event, which will call SetNode underneath and the panic occurred.

eggiter · 2021-08-31T08:20:09Z

@eggiter we need to fix func (r *Resource) Sub(rr *Resource) *Resource {

can you guys leave this fix to me? and you can tell me the idea of how to resolve if this PR can not achieve this puerpose.

eggiter · 2021-09-06T01:48:05Z

@k82cn @hzxuzhonghu what's the current status?

k82cn · 2021-09-07T01:21:06Z

the node is Ready but the allocatable gpu becomes '0'.

We should ignore such nodes during scheduling cycle.

eggiter · 2021-09-07T01:56:10Z

the node is Ready but the allocatable gpu becomes '0'.

We should ignore such nodes during scheduling cycle.

It will ignored by setting status of node to NotReady, which is done by this commit.

Thor-wl · 2021-09-07T02:19:47Z

I think such nodes can act as CPU nodes and continue to work if device plugin doesn't work well.

eggiter · 2021-09-07T02:28:55Z

I think such nodes can act as CPU nodes and continue to work if device plugin doesn't work well.

Sorry to tell you but, the machine can lost its memory occasionally. It'd be better to set status to NotReady and ignore in scheduling cycle.

Signed-off-by: lvhaodong <lvhaodong@kuaishou.com>

eggiter · 2021-10-21T01:57:03Z

@Thor-wl @k82cn PTAL!

k82cn · 2021-10-21T02:53:48Z

/lgtm
/approve

volcano-sh-bot · 2021-10-21T02:53:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/scheduler/OWNERS~~ [k82cn]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Thor-wl · 2021-10-21T05:56:15Z

/lgtm
/approve

volcano-sh-bot requested review from alcorj-mizar and merryzhou August 19, 2021 12:35

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 19, 2021

volcano-sh-bot assigned hzxuzhonghu, k82cn and william-wang Aug 19, 2021

eggiter force-pushed the fix-panic-when-setnode branch from fca3c3a to bea046b Compare August 19, 2021 12:41

Thor-wl requested review from wpeng102, hwdef, jasonliu747, shinytang6 and william-wang and removed request for merryzhou and alcorj-mizar August 20, 2021 02:41

Thor-wl reviewed Aug 23, 2021

View reviewed changes

volcano-sh-bot assigned Thor-wl Aug 23, 2021

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2021

volcano-sh-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 27, 2021

hzxuzhonghu reviewed Aug 28, 2021

View reviewed changes

eggiter requested a review from hzxuzhonghu August 31, 2021 02:50

eggiter requested a review from Thor-wl September 8, 2021 07:09

fix: fix possible panic when 'SetNode' is called

83a58c3

Signed-off-by: lvhaodong <lvhaodong@kuaishou.com>

eggiter force-pushed the fix-panic-when-setnode branch from bea046b to 83a58c3 Compare October 20, 2021 12:25

volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2021

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2021

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2021

Thor-wl removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2021

volcano-sh-bot merged commit 25d4f1f into volcano-sh:master Oct 21, 2021

eggiter deleted the fix-panic-when-setnode branch October 21, 2021 06:32

eggiter mentioned this pull request Nov 2, 2021

The Lost of GPUs problem has not been resolved completely #1818

Closed

eggiter mentioned this pull request Nov 17, 2021

Request to be a Member of Volcano community #1838

Closed

william-wang mentioned this pull request Jan 12, 2022

Large memory is used by volcano-scheduler #1947

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix possible panic when 'SetNode' is called #1685

fix: fix possible panic when 'SetNode' is called #1685

eggiter commented Aug 19, 2021

eggiter commented Aug 19, 2021

Thor-wl Aug 23, 2021

Thor-wl commented Aug 23, 2021

eggiter commented Aug 27, 2021

k82cn commented Aug 27, 2021

hzxuzhonghu Aug 28, 2021

eggiter Aug 30, 2021 •

edited

Loading

hzxuzhonghu Aug 31, 2021

eggiter Aug 31, 2021 •

edited

Loading

eggiter commented Aug 30, 2021 •

edited

Loading

hzxuzhonghu commented Aug 31, 2021

hzxuzhonghu commented Aug 31, 2021

k82cn commented Aug 31, 2021

eggiter commented Aug 31, 2021

eggiter commented Aug 31, 2021 •

edited

Loading

eggiter commented Sep 6, 2021

k82cn commented Sep 7, 2021 •

edited

Loading

eggiter commented Sep 7, 2021 •

edited

Loading

Thor-wl commented Sep 7, 2021

eggiter commented Sep 7, 2021

eggiter commented Oct 21, 2021

k82cn commented Oct 21, 2021

volcano-sh-bot commented Oct 21, 2021

Thor-wl commented Oct 21, 2021 •

edited

Loading

fix: fix possible panic when 'SetNode' is called #1685

fix: fix possible panic when 'SetNode' is called #1685

Conversation

eggiter commented Aug 19, 2021

eggiter commented Aug 19, 2021

Thor-wl Aug 23, 2021

Choose a reason for hiding this comment

Thor-wl commented Aug 23, 2021

eggiter commented Aug 27, 2021

k82cn commented Aug 27, 2021

hzxuzhonghu Aug 28, 2021

Choose a reason for hiding this comment

eggiter Aug 30, 2021 • edited Loading

Choose a reason for hiding this comment

hzxuzhonghu Aug 31, 2021

Choose a reason for hiding this comment

eggiter Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

eggiter commented Aug 30, 2021 • edited Loading

hzxuzhonghu commented Aug 31, 2021

hzxuzhonghu commented Aug 31, 2021

k82cn commented Aug 31, 2021

eggiter commented Aug 31, 2021

eggiter commented Aug 31, 2021 • edited Loading

eggiter commented Sep 6, 2021

k82cn commented Sep 7, 2021 • edited Loading

eggiter commented Sep 7, 2021 • edited Loading

Thor-wl commented Sep 7, 2021

eggiter commented Sep 7, 2021

eggiter commented Oct 21, 2021

k82cn commented Oct 21, 2021

volcano-sh-bot commented Oct 21, 2021

Thor-wl commented Oct 21, 2021 • edited Loading

eggiter Aug 30, 2021 •

edited

Loading

eggiter Aug 31, 2021 •

edited

Loading

eggiter commented Aug 30, 2021 •

edited

Loading

eggiter commented Aug 31, 2021 •

edited

Loading

k82cn commented Sep 7, 2021 •

edited

Loading

eggiter commented Sep 7, 2021 •

edited

Loading

Thor-wl commented Oct 21, 2021 •

edited

Loading